[Partitioner] User-defined partition #3237

beicy · 2019-07-16T07:14:25Z

Summary:
This PR added user-defined partition flow. Basically, a struct "PartitionConfig" containing the partition info is passed into Partitioner to enable this flow. Now we let users have the full control of how to do the partitioning.
To use this flow, users can write their helper function to generate PartitionConfig, and call Partitioner directly.
In the following PR, we will add passing PartitionConfig through HostManager from a yaml file.

Related to #2298
Documentation:

[Optional Fixes #issue]

Test Plan:
Added unittest. ninja test.

Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

lib/Partitioner/Partitioner.cpp

tests/unittests/PartitionerTest.cpp

bertmaher · 2019-07-16T21:11:56Z

lib/Partitioner/Partitioner.cpp

It doesn't look like fstream is used. (If it is, put it with the other #includes above.

bertmaher

I just have a handful of nits before landing. Thanks for prioritizing this :)

bertmaher · 2019-07-16T21:19:30Z

lib/Partitioner/Partitioner.cpp

Can this go before selectRepFunc (and maybe before getBackendMap)? It seems like it would circumvent the whole rest of the partitioning process so it'd be good to not do any irrelevant work.

will do it in front of selectRepFunc. However, we do need to run getBackendMap for later partition validation and logging.

bertmaher · 2019-07-16T21:20:45Z

lib/Partitioner/Partitioner.cpp

Only semi-related to this diff but I don't think (void)subF; is needed anymore, since subF is legitimately used below, even in Debug mode.

bertmaher · 2019-07-16T21:23:01Z

include/glow/Runtime/RuntimeTypes.h

sp: Paritition -> Partition

bertmaher · 2019-07-16T21:27:00Z

lib/Partitioner/Partitioner.cpp

nit: sp and inconsistent caps. // Validate memory usage. would read better.

bertmaher · 2019-07-16T21:27:22Z

lib/Partitioner/Partitioner.cpp

sp: validation

bertmaher · 2019-07-16T21:27:35Z

lib/Partitioner/Partitioner.cpp

sp: validation

artemrakhov-glow · 2019-07-16T22:18:33Z

include/glow/Runtime/RuntimeTypes.h

Suggested change

/// This is struct is for user defined partition.

/// This is struct for user defined partition.

artemrakhov-glow · 2019-07-16T22:18:51Z

include/glow/Runtime/RuntimeTypes.h

Suggested change

/// The name of the functiton to be partitioned.

/// The name of the function to be partitioned.

artemrakhov-glow · 2019-07-16T22:20:14Z

include/glow/Runtime/RuntimeTypes.h

Suggested change

/// The number of user defined partitions. The partition ids are [0,

/// The number of user defined partitions.

/// The partition ids are between 0 and numOfPartitions - 1, inclusive.

artemrakhov-glow · 2019-07-16T22:21:37Z

include/glow/Runtime/RuntimeTypes.h

Suggested change

llvm::StringMap<size_t> nodeToParitition;

llvm::StringMap<size_t> nodeToPartition;

artemrakhov-glow · 2019-07-16T22:25:27Z

include/glow/Runtime/RuntimeTypes.h

Is this a map between node's name and unsigned partition number? It wasn't clear to me initially that name is used here. Also, need to clarify that Glow mangles names, meaning that Glow's node name is never the same as name from Caffe2 protobuf.

artemrakhov-glow · 2019-07-16T22:37:14Z

lib/Partitioner/Partitioner.cpp

Suggested change

std::vector<NodesSet> nodesSets;

std::vector<NodesSet> nodesSets(partitionConfig_.numOfPartitions);

Then you won't need nodesSets.push_back(NodesSet{}); later.

artemrakhov-glow · 2019-07-16T22:38:36Z

lib/Partitioner/Partitioner.cpp

Suggested change

// If there is un used partition and un-mapped nodes, map those nodes to the

// If there is unused partition and unmapped nodes, map those nodes to the

artemrakhov-glow · 2019-07-16T22:39:16Z

lib/Partitioner/Partitioner.cpp

Please check that partitionID is < partitionConfig_.numOfPartitions

artemrakhov-glow · 2019-07-16T22:40:19Z

lib/Partitioner/Partitioner.cpp

Suggested change

// un-used partition.

// unused partition.

artemrakhov-glow · 2019-07-16T22:41:19Z

lib/Partitioner/Partitioner.cpp

Suggested change

assert(unUsed.size() && "Missing node to partition mapping.");

assert(unUsed.size() == 1 && "There must be exactly 1 unused partition.");

rdzhabarov · 2019-07-17T02:18:47Z

include/glow/Partitioner/Partitioner.h

doxygen style and above.

rdzhabarov · 2019-07-17T04:52:33Z

include/glow/Runtime/RuntimeTypes.h

is backendNames.size() == numOfPartitions. Why do we need numOfPartitions at all?

This means backendNames.size() should be equal to numOfPartitions. Otherwise, there is an error.

Can't we just have backendNames alone and the size tells how many partitions.

It also works. The reason I added this separate field is: I think it is clear for people just use a number directly as a reference. The we compare the size of partition names and backends in case something is missing by accident.

rdzhabarov · 2019-07-17T04:57:24Z

lib/Partitioner/Partitioner.cpp

please, add function name to the error message.

rdzhabarov · 2019-07-17T04:57:42Z

lib/Partitioner/Partitioner.cpp

rdzhabarov · 2019-07-17T04:59:20Z

lib/Partitioner/Partitioner.cpp

no asserts. DCHECK, add partition id to the error message.

rdzhabarov · 2019-07-17T04:59:45Z

lib/Partitioner/Partitioner.cpp

rdzhabarov · 2019-07-17T05:00:19Z

tests/unittests/PartitionerTest.cpp

nit: partitionFromConfig

facebook-github-bot

@beicy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gcatron · 2019-07-17T20:37:23Z

lib/Partitioner/Partitioner.cpp

  // we need the mapping between quantized tensor and original tensor.
  FunctionToBackendNameMap funcToBackend;
  funcToBackend = backendBasedPartition(F_, backends, cctx);
  module_->eraseFunction(F_);


Is it possible that the function can be renamed and will no longer match the name in partitionConfig?

No. If partitionConfig is set, Partitioner will go to user-defined flow without considering other options like "-load-profiling". So later renaming in the rest flow won't affect the function name.

artemrakhov-glow · 2019-07-17T21:28:32Z

include/glow/Runtime/RuntimeTypes.h

+  /// mapped to this partition id are not in this map, and the nodes mapped to
+  /// other partitions ids must be in this map. The node's name should be the
+  /// name in Glow function and may be different from the original name from
+  /// models. Since Glow will mangle names to make them unique.


It's not only to make them unique. We also e.g. replace / with _. Maybe something else.
You may write: Since Glow will mangle names according to its own conventions.

artemrakhov-glow · 2019-07-17T21:36:31Z

include/glow/Partitioner/Partitioner.h

You can see that it's already bad API. Let's say caller 1 has only partitionConfig. But now it has to pass false, false for some other 2 flags. Caller 1 has to figure out what is saturation, and what to pass for corresponding flag to disable.

It would be a good practice to make sure that incompatible flags are not passed together. E.g. saturation=false when user defined partition. But these checks can quickly explode if we have more flags/modes.

What if we later introduce another feature, third mode for partitioner? Say, PyTorch-defined partition? Then caller 3 will have to figure out what to pass for partitionConfig argument to disable user-defined partition feature.

This is why I think we should never accept a whole all-inclusive list of args in one function (in this case, partitioner constructor). I was going to propose create different method for user defined partition. But if you are going to refactor everything later into different classes, I'm also fine with that.

artemrakhov-glow · 2019-07-17T21:39:15Z

lib/Partitioner/Partitioner.cpp

+      partitionMap.add(node, funcList[partitionID]);
+      nodesSets[partitionID].insert(node);
+    }
+  }


else {
what if there are unused partitions? Meaning that some partitions are empty. Is it valid?

There can be at most 1 unused partition (there is the check). Otherwise, it is invalid. Since it means we have empty functions in the module and will cause problems later.

will cause problems later

It is better to nail these problems early. Here we still can report what exactly is wrong: no nodes were mapped to a partitions #i, #j, #k... Later it will be difficult to track down where the empty function is coming from. In fact, it may fail with obscure error, so first debugging step would be to figure out that the function is empty, and only after that find out why it's empty. I really suggest to report as many problems with the partition as we can find.

Yes, I agree. So far, if there is empty function, it will cause the assertion and stop. But you are right, the error message is not enough. Will improve the debugging ability. Thanks!

gcatron

LGTM Great work!

beicy · 2019-07-17T22:37:27Z

@artemrakhov Thanks a lot for the suggestion on the interface. Let's have more discussion about the interface offline.

facebook-github-bot · 2019-07-18T01:03:22Z

@beicy merged this pull request in 809a3e8.

facebook-github-bot added the CLA Signed label Jul 16, 2019

beicy force-pushed the auto1 branch from e1d9e26 to c2ca40b Compare July 16, 2019 07:16

rdzhabarov reviewed Jul 16, 2019

View reviewed changes

lib/Partitioner/Partitioner.cpp Outdated Show resolved Hide resolved

beicy force-pushed the auto1 branch from c2ca40b to 2054a24 Compare July 16, 2019 17:23

rdzhabarov reviewed Jul 16, 2019

View reviewed changes

tests/unittests/PartitionerTest.cpp Outdated Show resolved Hide resolved

bertmaher reviewed Jul 16, 2019

View reviewed changes

beicy force-pushed the auto1 branch from 2054a24 to 4dcceeb Compare July 16, 2019 21:29

bertmaher suggested changes Jul 16, 2019

View reviewed changes

artemrakhov-glow suggested changes Jul 16, 2019

View reviewed changes

beicy force-pushed the auto1 branch 2 times, most recently from bfb931b to c21d412 Compare July 16, 2019 23:14

rdzhabarov reviewed Jul 17, 2019

View reviewed changes

beicy force-pushed the auto1 branch from c21d412 to 212196b Compare July 17, 2019 07:18

[Partitioner] User-defined partition

6637109

beicy force-pushed the auto1 branch from 212196b to 6637109 Compare July 17, 2019 19:23

bertmaher approved these changes Jul 17, 2019

View reviewed changes

facebook-github-bot reviewed Jul 17, 2019

View reviewed changes

gcatron reviewed Jul 17, 2019

View reviewed changes

artemrakhov-glow reviewed Jul 17, 2019

View reviewed changes

artemrakhov-glow approved these changes Jul 17, 2019

View reviewed changes

gcatron approved these changes Jul 17, 2019

View reviewed changes

facebook-github-bot closed this in 809a3e8 Jul 18, 2019

facebook-github-bot added the Merged label Jul 18, 2019

beicy deleted the auto1 branch July 25, 2019 22:08

	/// This is struct is for user defined partition.
	/// This is struct for user defined partition.

	/// The name of the functiton to be partitioned.
	/// The name of the function to be partitioned.

	/// The number of user defined partitions. The partition ids are [0,
	/// The number of user defined partitions.
	/// The partition ids are between 0 and numOfPartitions - 1, inclusive.

	llvm::StringMap<size_t> nodeToParitition;
	llvm::StringMap<size_t> nodeToPartition;

	std::vector<NodesSet> nodesSets;
	std::vector<NodesSet> nodesSets(partitionConfig_.numOfPartitions);

	// If there is un used partition and un-mapped nodes, map those nodes to the
	// If there is unused partition and unmapped nodes, map those nodes to the

	assert(unUsed.size() && "Missing node to partition mapping.");
	assert(unUsed.size() == 1 && "There must be exactly 1 unused partition.");

[Partitioner] User-defined partition #3237

[Partitioner] User-defined partition #3237

Uh oh!

Conversation

beicy commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bertmaher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beicy Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beicy Jul 17, 2019 •

edited

Loading